small enhancements to Task #13963

vtjnash · 2015-11-13T06:04:52Z

This is the simpler stuff (3 independent items) that I pulled out of #13099:

move the call to throw_internal outside of finish_task. this makes certain error conditions slightly more reliable when stack is limited (and thus also should make errors in finish_task normal-fatal rather than segfault-fatal)
delete task->last. it wasn't necessary for anything, and so it just adds complexity and delays gc
fix the no copy-stacks build option

if throw_internal is running from the segv_handler, there might not be enough stack to run arbitrary functions this change, therefore, makes stack overflow in tasks more reliable

the task->last field was unnecessary, and could result in a task staying alive in the gc unnecessarily :runnable was almost equivalent to :waiting, except that it might "be restarted unpredictably" (via the task->last field)

tkelman · 2015-11-13T08:37:24Z

only ca9d5e7 for the backport label, right?

vtjnash · 2015-11-16T17:00:49Z

@tkelman yes

seeing there has been no objections, i'll merge this in a few hours

small enhancements to Task

JeffBezanson · 2015-11-17T04:48:15Z

base/exports.jl

@@ -1010,7 +1010,6 @@ export
    Condition,
    consume,
    current_task,
-    istaskstarted,


This removes an exported function without a deprecation. Is there now no way to tell that a task has started?

C should be able to still compute this if needed

Then we should have this function call such C code, and then put a proper deprecation warning on it if we're actually going to remove it. I don't see why though. Is there a good reason to remove it?

In various intermediate versions of the PR, it wasn't possible, so I eliminated it. Later on, I didn't see an obvious need to reimplement it. Plus, the only usage of it in Base (in serialize) appears to be a race condition.

restored in 07ef29f

JeffBezanson · 2015-11-17T04:54:14Z

I don't understand the claim that the waiting state is "almost the same as" the runnable state. In the waiting state, a task cannot be restarted because it's waiting for an event. In the runnable state, that event has happened, and the task could be run if chosen by a scheduler.

Also, task states are mentioned in the control flow chapter of the manual so that needs to be updated if we keep this change.

JeffBezanson · 2015-11-17T04:59:09Z

base/task.jl

-        # if a finished task accidentally gets into the queue, wait()
-        # could return. in that case just take the next task off the queue.
-        while true
-            wait()


Are you sure this loop is no longer necessary?

yes. calling yieldto on this task will cause jl_switchto to call jl_throw

vtjnash · 2015-11-17T05:15:57Z

In the runnable state, that event has happened, and the task could be run if chosen by a scheduler.

that would be the :queued state. in the runnable state, no event had happened, but the task will be inserted into the scheduler if task_done_hook finds task.last is in that state.

JeffBezanson · 2015-11-17T05:24:04Z

Ok, in that case maybe the :runnable state in this PR should be renamed to :waiting. It's confusing to me that a task that's waiting for an event, and therefore can't run, should be marked "runnable". Then the currently-running task should perhaps be marked as :running. Even though this is redundant with simply checking current_task(), it seems much clearer to me.

JeffBezanson · 2015-11-17T05:25:40Z

base/serialize.jl

@@ -348,13 +348,13 @@ end

 function serialize(s::SerializationState, t::Task)
    serialize_cycle(s, t) && return
-    if istaskstarted(t) && !istaskdone(t)
+    if !istaskdone(t)


With the old version of this code, it was possible to make a task and then serialize it right away (a slightly strange thing to do, but it could fall out of reasonable code quasi-accidentally). Is that no longer possible?

Sure, I just don't see how it could safely fall out of reasonable code. This condition introduces behavior whereby you can create copies of a Task, but only until it starts running. However, since the next few lines call serialize (and thus yield), the Task could start running while this function is going leading to inconsistent serialization results. (I realize the same could be said for all of the serialize methods, since we don't buffer / deepcopy the whole tree first, but perhaps such is life)

I think there are ways of avoiding this, depending on the desired behavior. A stable result is guaranteed once the task is finished, however, making that state the only state that is definitely safe to serialize.

vtjnash · 2015-11-17T05:48:36Z

Even though this is redundant with simply checking current_task(), it seems much clearer to me.

The new states aren't necessary all that similar to the old states: :queued means "owned by the scheduler queue (not valid as a call to schedule or yield)", :runnable means "not owned by the scheduler queue (valid as a call to schedule or yield)". The istaskdone states are unchanged.

it's certain possible to split :runnable into :running and :notrunning (which would be similar to the old :waiting state). I don't see it as worth that extra bit of effort anytime a piece of code wants to change states (plus the need then to account for possible inconsistency and try/catch).

JeffBezanson · 2015-11-17T05:58:04Z

Ok, that's all fine, but "runnable" is just not the right word to describe a task that's blocked on an event.

vtjnash · 2015-11-17T06:08:38Z

If :runnable means "can be scheduled to run", then that's exactly what it means. Even when it is blocked on an event, it is still ready to be scheduled to run. Equivalently, :runnable means "not already queued in the scheduler nor already finished".

JeffBezanson · 2015-11-17T06:31:38Z

The way a scheduler is traditionally described is that you have a set of tasks or threads. Some subset of those are waiting for external events and therefore cannot run. The rest are "runnable", and the scheduler picks something from that set to actually run. The set of tasks the scheduler might pick from is obviously important, so it makes sense to have a label for it.

A crucial distinction is whether the scheduler is the reason the task is not currently running. That can be fixed or changed using a different scheduling algorithm. But if you're waiting for an outside event, the scheduler can't control whether you run.

In ordinary conversation, if a task is blocked on I/O wouldn't you say it's "waiting" or "blocked"?

vtjnash · 2015-11-17T06:49:10Z

Yes, I simply still do not see the point is distinguishing between :running and :waiting. That just requires more work on the part of the implementer to make sure they stay synchronized with reality.

A crucial distinction is whether the scheduler is the reason the task is not currently running

Exactly. When the scheduler is the reason a task is not running, the state is :queued (aka "scheduled"). The other possible states are finished and runnable. The scheduler doesn't care why the task is not currently queued, so it all gets categorized as :runnable. In ordinary conversation, the tasks that are waiting for an event should not appear anywhere else.

JeffBezanson · 2015-11-17T06:59:36Z

I don't know, I would argue that a task that's currently running and one that's waiting for I/O have meaningfully different statuses. If we were looking at a task in a debugger wouldn't that be an important distinction? In your last sentence you wrote "...tasks that are waiting for an event...". So why not call that state "waiting"?

vtjnash · 2015-11-17T07:17:12Z

because it adds unnecessary state to the system and you still haven't given a use case for it. i doubt i would use this in a debugger, since it won't necessarily return the most accurate or useful answer, whereas the debugger can quickly and accurately test jl_current_task and check the backtrace to print the right answer.

JeffBezanson · 2015-11-17T07:29:57Z

I believe this was a use case:

julia/base/task.jl

Line 156 in 34a834f

active_repl_backend.backend_task.state == :waiting && isempty(Workqueue) &&

StefanKarpinski · 2015-11-17T15:47:13Z

@vtjnash – calling a blocked task "runnable" is confusing and wrong. Eliminating a state is fine, but you can't call blocked tasks runnable.

vtjnash · 2015-11-17T16:46:47Z

"blocked" is not a task state that is managed by the scheduler. While a blocked task is both runnable (not scheduled, but it is still executable) and waiting. You can change the state of a task to whatever you want when it is not in the scheduler; the system really couldn't care less. However, to put it in the scheduler, it must be in the :runnable state. If you want to add code to that sets some other state (say "oatmeal"), you'll just need to also add more code to change it back before running the task. Because nothing else anywhere in the system cares about the state of the task other than our current Scheduler, nothing else bothers with trying to manage that state. However, you're welcome to add try-catch blocks in every wait and notify implementation to manage the updating of this field if you so choose.

StefanKarpinski · 2015-11-17T17:04:31Z

This can't be called "runnable" since it's often not actually runnable. Come up with a better name.

if throw_internal is running from the segv_handler, there might not be enough stack to run arbitrary functions this change, therefore, makes stack overflow in tasks more reliable (cherry picked from commit ca9d5e7) ref #13963

tkelman · 2015-12-07T13:11:50Z

Name bug is still open here, yeah?

vtjnash added 3 commits November 12, 2015 23:34

avoid running finish_task inside of throw_internal

ca9d5e7

if throw_internal is running from the segv_handler, there might not be enough stack to run arbitrary functions this change, therefore, makes stack overflow in tasks more reliable

delete task->last and the :waiting state

ac02379

the task->last field was unnecessary, and could result in a task staying alive in the gc unnecessarily :runnable was almost equivalent to :waiting, except that it might "be restarted unpredictably" (via the task->last field)

fix no COPY_STACKS option

b238ac2

vtjnash changed the title ~~enhancements to Task~~ small enhancements to Task Nov 13, 2015

vtjnash added the backport pending 0.4 label Nov 13, 2015

vtjnash added a commit that referenced this pull request Nov 16, 2015

Merge pull request #13963 from JuliaLang/jn/task_enhancements

ab9389c

small enhancements to Task

vtjnash merged commit ab9389c into master Nov 16, 2015

vtjnash deleted the jn/task_enhancements branch November 16, 2015 20:07

JeffBezanson reviewed Nov 17, 2015
View reviewed changes

tkelman mentioned this pull request Nov 18, 2015

Build fails with JULIA_THREADS=1 #14028

Closed

tkelman removed the backport pending 0.4 label Dec 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

small enhancements to Task #13963

small enhancements to Task #13963

vtjnash commented Nov 13, 2015

tkelman commented Nov 13, 2015

vtjnash commented Nov 16, 2015

JeffBezanson Nov 17, 2015

vtjnash Nov 17, 2015

JeffBezanson Nov 17, 2015

vtjnash Nov 17, 2015

vtjnash Nov 17, 2015

JeffBezanson commented Nov 17, 2015

JeffBezanson Nov 17, 2015

vtjnash Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

JeffBezanson Nov 17, 2015

vtjnash Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

StefanKarpinski commented Nov 17, 2015

vtjnash commented Nov 17, 2015

StefanKarpinski commented Nov 17, 2015

tkelman commented Dec 7, 2015

small enhancements to Task #13963

small enhancements to Task #13963

Conversation

vtjnash commented Nov 13, 2015

tkelman commented Nov 13, 2015

vtjnash commented Nov 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Nov 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

vtjnash commented Nov 17, 2015

JeffBezanson commented Nov 17, 2015

StefanKarpinski commented Nov 17, 2015

vtjnash commented Nov 17, 2015

StefanKarpinski commented Nov 17, 2015

tkelman commented Dec 7, 2015